Skip to content

Optimization: Use CollectionsMarshal.GetValueRefOrAddDefault() for Dictionary operations#487

Open
SergeiPavlov wants to merge 1 commit into
master-servicetitanfrom
GetValueRefOrAddDefault
Open

Optimization: Use CollectionsMarshal.GetValueRefOrAddDefault() for Dictionary operations#487
SergeiPavlov wants to merge 1 commit into
master-servicetitanfrom
GetValueRefOrAddDefault

Conversation

@SergeiPavlov
Copy link
Copy Markdown
Collaborator

No description provided.

@SergeiPavlov SergeiPavlov requested a review from snaumenko-st May 13, 2026 21:26
Copy link
Copy Markdown

@snaumenko-st snaumenko-st left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this unsafe pattern shouldn't be used everywhere just because we can. The only place that it's worth it - real bottlenecks, identified by profiler or at least isolated library code.

@SergeiPavlov
Copy link
Copy Markdown
Collaborator Author

SergeiPavlov commented May 13, 2026

I believe this unsafe pattern

Could you give an example of misusing of this pattern?
What is the danger?

at least isolated library code.

The DataObjects is such isolated library code
I agree that this should not be used in the App's Modules

@SergeiPavlov SergeiPavlov requested a review from botinko May 13, 2026 22:02
Copy link
Copy Markdown

@botinko botinko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is low-level optimization. It worth adding it to hot-path only, not as a defult option. Let's remove it from ModelBuilder, and other non-hot path places.

@SergeiPavlov
Copy link
Copy Markdown
Collaborator Author

SergeiPavlov commented May 13, 2026

Let's remove it from ModelBuilder, and other non-hot path places.

I consider all DO code as performance-critical.
Model building takes significant amount of time and we are interested to reduce it

@botinko
Copy link
Copy Markdown

botinko commented May 13, 2026

I consider all DO code as performance-critical.
Model building takes significant amount of time and we are interested to reduce it

I'm not disagree, but to be honest nano-second optimization does nothing (0.000001%) in this direction.

@SergeiPavlov Optimization has no real bechmarks / perf traces / other provable justification. In such cases I accept only 100% safe + code quality-improving changes OR obviously impactful optimizations. From my perspective this is a insignificant nano-second optimizations, that complicates the code without any justification.

Current approach of optimization of low-hanging fruites or by the method of the intense gaze is unfocused and unligned with real behaviour on prod. Optimizations based on performance profiling traces will amplify your impact X100.

@SergeiPavlov
Copy link
Copy Markdown
Collaborator Author

SergeiPavlov commented May 13, 2026

Current approach of optimization of low-hanging fruites

This approach typically is the most effective

@snaumenko-st
Copy link
Copy Markdown

snaumenko-st commented May 14, 2026

I believe this unsafe pattern

Could you give an example of misusing of this pattern? What is the danger?

The danger is that operations over references are not considered safe in .NET in general. It's hard to forget about checking them for null, and references should be treated in a special way using Unsafe class. It's already enough. The other thought is that DO is not something that we own currently e2e, and do backmerges from upstream regularly. This can break things as well.

at least isolated library code.

The DataObjects is such isolated library code I agree that this should not be used in the App's Modules

By isolated I mean a single isolated unsafe class or helper, which is not changing frequently and itself has bounded safe context of application, not spread across the entire codebase.

The same idea is why we don't use Span manipulations everywhere - it adds unnecessary complexity to the code without noticeable impact.

@SergeiPavlov
Copy link
Copy Markdown
Collaborator Author

It's hard to forget about checking them for null,

This is not more dangerous than forgetting for null-checking of any reference.
We have a risk to get NullReferenceException

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants